fix(discovery): resolve all native user-scope surfaces from the agent directory (#4769) - #4784
fix(discovery): resolve all native user-scope surfaces from the agent directory (#4769)#4784Yeachan-Heo wants to merge 1 commit into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f0ccee410
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| */ | ||
| function resolveUserAgentDir(ctx: LoadContext): string { | ||
| return ctx.userAgentDir ?? getAgentDir(); | ||
| return path.resolve(ctx.userAgentDir ?? getAgentDir()); |
There was a problem hiding this comment.
When createAgentSession({ agentDir }) selects a profile different from the process-wide getAgentDir(), this fallback still selects the process-wide profile because sdk/session.ts invokes loadSkills, rule discovery, and context-file discovery with only cwd, while the system-prompt loaders also expose no agentDir option. The session can therefore omit the requested profile's skills/instructions and inject SYSTEM.md, RULES.md, AGENTS.md, or skills from another profile; pass the resolved session agentDir through all capability-based startup readers rather than relying on this fallback.
Useful? React with 👍 / 👎.
| name: string, | ||
| policy?: SkillsSettings, | ||
| home = getRuntimeHome(), | ||
| agentDir = getAgentDir(), |
There was a problem hiding this comment.
For a session whose explicit agentDir differs from the process-wide directory, SkillDiscoveryTool now advertises skills from the session profile, but SkillTool still calls findRuntimeSkillByName(this.#session.cwd, requestedName, policy) without this new argument. If the skill is not already in the startup snapshot—for example, it was installed during the session—the subsequent invocation searches the global profile and reports the advertised skill as unavailable; thread getSessionAgentDir() into this lookup.
Useful? React with 👍 / 👎.
|
Verification evidence from the originating issue lane (same 22-file change set, original head
Negative control (assertions are not environment-dependent)Reverting only the source diff (tests kept) makes 6/7 of Pre-existing local red, proven unrelated (identical with the diff stashed)
Fail-closed confirmations
Local suites at the original head (all green)
Consolidation audit (no duplicate-lane drift)This change set supersedes and fully contains the Issue-lane handoff (issue #4769 → this PR): ownership of mutation/rebase/review/merge is with |
Keep terminal safety-stop authority exclusive to first-party adapter parse sites and sanitize the trailing completion path before managed-shell rebuilding. Public AI consumers can verify or transfer existing provenance but cannot mint it from message data or structural signals. Fixes the two P1 review findings on #4777. Lore-id: 4777-p1-provenance Constraint: custom provider streams must remain fallback-eligible unless adapter provenance is authenticated Constraint: user cancellation and non-error completion must never become provider safety stops Rejected: public mint helper | arbitrary consumers could forge allowlisted terminal authority Confidence: high Scope-risk: regression-risk Reversibility: revert-commit Tested: focused provider, managed-attempt, session-retry, and provider-safety e2e suites Not-tested: full CI after sibling #4784 merges
|
CI triage for EvidenceI reproduced locally on both the pristine pre-change base ( Why it failsThese tests isolate the user scope by setting only The repair the PR lane should apply (mechanical, same pattern as the suites already updated in this PR)In each affected test, after const originalAgentDir = getAgentDir();
setAgentDir(path.join(home, ".gjc", "agent"));
// … finally: setAgentDir(originalAgentDir)Two tests need slightly more because they deliberately set
Two template/contract failures (also for the PR lane)
|
8f0ccee to
b72820c
Compare
|
Exact-head human gate for PR #4784:
The exact remote #4784 delta was transplanted byte-for-byte, and the follow-up fixes session home/agent-dir injection through actual tool construction so |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b72820cb99
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| getActiveSkillPhase: () => session?.getActiveSkillPhase(), | ||
| getDeepInterviewAskStage: () => session?.getDeepInterviewAskStage(), | ||
| getHindsightSessionState: () => session?.getHindsightSessionState(), | ||
| getSessionAgentDir: () => agentDir, |
There was a problem hiding this comment.
When createAgentSession receives profile-scoped settings without a separate agentDir, this accessor returns the process-wide directory captured in agentDir rather than settings.getAgentDir(). This is the path used by canonical subagents: task/executor.ts passes settings: subagentSettings but no agentDir. Consequently, their skill_discovery and runtime skill fallback scan the default profile, omitting the parent's profile skills and potentially advertising skills from another profile. Use the injected settings directory when options.agentDir is absent.
Useful? React with 👍 / 👎.
|
@probepark @snowykr please review exact head |
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The five-axis review completed against the exact head and identified 3 actionable issues, led by Custom-home runtime discovery ignores the supplied home and Honor the supplied home when resolving the default agent directory. These findings require changes before approval.
Findings / Required Changes
- [P1] Custom-home runtime discovery ignores the supplied home.
Reference:packages/coding-agent/src/extensibility/runtime-skill-discovery.ts:349-350
Whenhomeis provided withoutagentDir, discovery defaults to process-globalgetAgentDir()instead of deriving the default agent directory from that home, so injected/test homes and SDK callers can miss their user skills. Derive the default agent directory from the supplied home or preserve the prior home-relative behavior. - [P1] Honor the supplied home when resolving the default agent directory.
Reference:packages/coding-agent/src/extensibility/runtime-skill-discovery.ts:349-356
When callers provideoptions.homebut omitagentDir, discovery falls back to process-globalgetAgentDir()rather than<home>/.gjc/agent; custom-home callers can therefore read another profile's skills, and concurrent profile changes can select the wrong scope. Derive the default agent directory from the supplied home or require callers to pass both values consistently. - [P2] Document configurable user paths consistently.
Reference:docs/customization.md:45-47
The authority contract and skill documentation identify the configurable agent directory, but the MCP and Hook rows still present ~/.gjc/agent as universally authoritative. Update these rows to use <agentDir> with the default path noted, matching the agent-directory profile behavior.
CI / Verification
- Reviewed the exact remote head:
b72820cb996af0ed19bd17a43bef6342b807b04a. - CI summary: 34 passing, 3 failing, 11 pending/cancelled/skipped.
- Failing checks:
Validate exact-head PR contract,Validate exact-head PR contract,PR contract bootstrap. - Repository policy permits review before all gating checks pass; the current non-passing checks are recorded above and do not establish that checks passed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | APPROVED | Agent-directory profile isolation and legacy default-profile compatibility are established without an actionable API boundary defect. |
| A2. Architecture / Correctness / Failure | CHANGES_REQUESTED | Runtime skill discovery correctly propagates explicit agent directories, but omitted agentDir with a supplied home can cross profile boundaries; no additional concurrency defect was established. |
| A3. Security / Privacy / Trust | APPROVED | Security and privacy risk from agent-directory skill and configuration discovery was reviewed; no actionable regression was established. |
| A4. Verification / Tests / CI | CHANGES_REQUESTED | A4 review establishes passing affected tests and CI, with one observable custom-home discovery regression and unresolved PR-contract validation. |
| A5. Context / Compatibility / Platform | CHANGES_REQUESTED | A1/A5 integration and platform documentation is mostly aligned, but MCP and Hook path references remain stale for configured agent-directory profiles. |
Limitations
- PR contract checks failed, so merge-contract compliance is not established; affected validation and tests passed.
- PR contract checks failed, but ci_summary exposes no failure reason, so exact contract noncompliance cannot be assessed.
- PR contract checks failed, so contract validation is not treated as passing evidence.
- Windows native and darwin-arm64 platform jobs were skipped, so platform behavior on those targets was not verified by CI.
|
#4768 author here (the MCP half of #4769's parent). This carries the remaining five surfaces the way the review asked, and the One review item from #4768 is still open, and this PR is the last cheap moment for it. @probepark's approving review there ended with:
This PR keeps I measured the cost on your exact head
16 of 24 are inside files this PR already rewrites, and the 8 others are mechanical — Not blocking from me — I do not own this lane and I am not the reviewer of record. But the request was explicit and the mechanism is being finalized in this diff, so it should be a deliberate accept-or-decline rather than something that falls off the edge of two PRs. (For the record on the other half: #4768's docs deliberately claimed only "discovery, |
|
Reviewer follow-up is complete at exact head Addressed snowykr's three findings:
Please perform a fresh exact-head review and approve if satisfied. The PR remains |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92d7c4f38e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| policy?: SkillManagementPolicy; | ||
| }): Promise<ManagedSkillRecord[]> { | ||
| const home = options.home ?? getRuntimeHome(); | ||
| const agentDir = options.agentDir ?? getAgentDir(); |
There was a problem hiding this comment.
When callers supply the existing home override without the new agentDir option, this now scans the process-wide profile instead of <home>/.gjc/agent; exported SDK consumers and loadCustomizationInventory tests using an isolated home can therefore omit that home's user skills and expose skills from the real profile. Preserve the prior home-only behavior by deriving the default agent directory from an explicitly supplied home, as discoverRuntimeSkills now does.
Useful? React with 👍 / 👎.
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
First review at exact head 92d7c4f3 — merge blocked. This is real progress on #4767 and I measured it fixing a suite that is currently red on dev, but the session-scoped call sites were not threaded and the test that appears to cover that performs the missing step itself.
measured benefit, not just claimed
I ran the currently-red suites against this head with a real install and natives built:
| suite | on dev ba7096f7 |
on this head |
|---|---|---|
skills.test.ts |
31 pass / 5 fail | 36 pass / 0 fail |
gjc-plugin-no-surface.test.ts |
3 / 1 | 3 / 1 |
coordinator-mcp/stop-session.test.ts |
5 / 6 | 5 / 6 |
So this genuinely closes the skills half of the ambient-home problem. The other two remain red and are not this PR's job.
the trust boundary holds
I checked this first, because a discovery-path change is where a checkout could gain authority. loadCapability selects an explicit caller agentDir or trusted getAgentDir() (capability/index.ts:239-242); project .env overrides are rejected by trustedValue (utils/src/dirs.ts:222-230, :429-432); and Settings.agentDir is constructor state rather than config.yml data (config/settings.ts:544-547). No project-level setting gains a say.
major — session-scoped profiles write to one place and read from another
sdk/session.ts:1288 captures the session's authority:
const agentDir = options.agentDir ?? getDefaultAgentDir();Startup discovery does not use it (:1408-1411):
: logger.time("discoverContextFiles", loadContextFilesResultInternal, { cwd });Only cwd. So a session created with an explicit agentDir writes settings and skills under that profile while loading skills, RULES.md, AGENTS.md, SYSTEM.md and the shared native capability surfaces through the process-wide fallback. That is the same write/read divergence #4767 is about, surviving for session scope precisely because this PR made the readers consume LoadContext.userAgentDir without updating the session call sites that must supply it.
And the test cannot see it. test/issue-4769-user-scope-agent-dir.test.ts:144-169 calls setAgentDir(profile) before loadSkills, which makes the global resolution agree with the session profile — so the divergence is masked by the setup. That is a test performing the missing production step itself: it proves the mechanism can work, not that the session path uses it.
The regression that would catch it: createAgentSession({ agentDir }) while the global getAgentDir() points elsewhere, asserting every startup reader resolves to the session profile.
coverage gaps worth closing with it
Positive pins exist for config, SYSTEM.md, skills, RULES.md and AGENTS.md (test/discovery/builtin-user-agent-dir.test.ts:65-100), and old-path negative controls exist for SYSTEM.md, RULES.md and skills (issue-4769-user-scope-agent-dir.test.ts:76-113) — but not for AGENTS.md, config.yml, or the shared getConfigDirs consumers. Decoys for every migrated surface would make the migration self-guarding.
pre-existing caveat, not attributed here
capability/fs.ts:11-38 collapses every readFile/readDirEntries error into cached absence, and discovery/builtin.ts:67-92 treats an unreadable profile as empty. So a permissions error on a user profile reads as "no configuration" rather than surfacing. Untouched by this PR, worth its own change.
Reviewed by @probepark — method: verified the trust boundary before assessing the fix, measured the currently-red suites against this head with a real install rather than my symlinked worktree, then traced the captured session agentDir into the startup discovery calls and checked what the covering test sets up before asserting.
gajae.pr-review-verdict.v1 merge-blocked sha256:ebfb02cc7ff17126869d7e938dc3719533272a0a7463385bd8186200b3e730d0 reviewer:human reviewer-id:probepark evidence:exact-head-92d7c4f3-fixes-skills-suite-36-0-but-session-scoped-agentdir-is-captured-and-not-threaded-into-startup-discovery
Keep terminal safety-stop authority exclusive to first-party adapter parse sites and sanitize the trailing completion path before managed-shell rebuilding. Public AI consumers can verify or transfer existing provenance but cannot mint it from message data or structural signals. Fixes the two P1 review findings on #4777. Lore-id: 4777-p1-provenance Constraint: custom provider streams must remain fallback-eligible unless adapter provenance is authenticated Constraint: user cancellation and non-error completion must never become provider safety stops Rejected: public mint helper | arbitrary consumers could forge allowlisted terminal authority Confidence: high Scope-risk: regression-risk Reversibility: revert-commit Tested: focused provider, managed-attempt, session-retry, and provider-safety e2e suites Not-tested: full CI after sibling #4784 merges
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The five-axis review completed against the exact head and identified 5 actionable issues, led by Ignored home override breaks skill-management API compatibility and Injected home is ignored for skill-management user paths. These findings require changes before approval.
Findings / Required Changes
- [P1] Ignored home override breaks skill-management API compatibility.
Reference:packages/coding-agent/src/extensibility/skill-management.ts:145
resolveNativeSkillScopeDir previously used its third home argument for default user writes, but it is now ignored and user writes use the process-wide agent directory. Preserve the established home-based behavior when agentDir is omitted, while allowing explicit agentDir profiles. - [P1] Injected home is ignored for skill-management user paths.
Reference:packages/coding-agent/src/extensibility/skill-management.ts:145-150,176
resolveNativeSkillScopeDirandlistNativeSkillsForManagementdefaultagentDirto process-globalgetAgentDir()even whenhomeis explicitly supplied. User writes/listing for an injected profile can target or read another profile, especially concurrently. Derive the default agent directory from the injected home, or require/pass agentDir consistently, and add isolation tests for omitted agentDir. - [P1] Injected profile home can expose global user skills.
Reference:packages/coding-agent/src/extensibility/skill-management.ts:179
listNativeSkillsForManagement derives agentDir from process-global getAgentDir() when home is injected but agentDir is omitted, potentially scanning another profile's skills. Derive the default agent directory from the injected home or require an explicit agentDir. - [P1] Injected-home skill writes use the process-global profile.
Reference:packages/coding-agent/src/extensibility/skill-management.ts:145-151
resolveNativeSkillScopeDir ignores its supplied home and defaults agentDir to getAgentDir(), so writeNativeSkill({home}) can write outside the injected profile while discovery resolves that profile’s agent directory; derive the default agent directory from the supplied home or require and consistently pass agentDir. - [P1] Missing ToolSession accessor.
Reference:packages/coding-agent/src/tools/skill.ts:133
The skill tool calls getSessionAgentDir(), but ToolSession does not declare that member. Add the optional accessor to ToolSession (or remove the call) and ensure session construction satisfies the interface.
CI / Verification
- Reviewed the exact remote head:
92d7c4f38ea1fac7dcf2901551d63d7a16289606. - CI summary: 34 passing, 2 failing, 12 pending/cancelled/skipped.
- Failing checks:
Validate exact-head PR contract,PR contract bootstrap. - Repository policy permits review before all gating checks pass; the current non-passing checks are recorded above and do not establish that checks passed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | CHANGES_REQUESTED | Profile-based skill discovery and writing are mostly aligned, but the session accessor contract and backward-compatible home override remain concrete API risks. |
| A2. Architecture / Correctness / Failure | CHANGES_REQUESTED | Correctness is mostly coherent, but injected-home skill-management calls can violate profile isolation and produce cross-profile results under concurrency. |
| A3. Security / Privacy / Trust | CHANGES_REQUESTED | Profile isolation is enforced in runtime discovery, but skill-management listing can cross profile boundaries when callers inject home without agentDir. |
| A4. Verification / Tests / CI | APPROVED | A4 review found passing affected tests and no established regression; CI contract failures remain an unresolved verification risk. |
| A5. Context / Compatibility / Platform | CHANGES_REQUESTED | Integration and platform review found a profile-isolation risk in the skill writer; documentation changes otherwise align the advertised agent-directory model. |
Limitations
- The PR contract and bootstrap CI checks are failed, so exact-head contract validation could not be established; affected-path tests and virtual integration validation succeeded.
|
Final fix-forward exact head is ready for a fresh review:
Addressed both reviewers' requested changes: actual session startup threading, home-relative skill-management list/write resolution with concurrent isolation, and negative decoys for AGENTS/config/shared config-directory consumers. Affected tests and coding-agent package check pass. Please review and approve this exact head if satisfied. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ebd7b06ea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ): Promise<ProjectContextFilesResult> { | ||
| const resolvedCwd = options.cwd ?? getProjectDir(); | ||
| const result = await loadCapability(contextFileCapability.id, { cwd: resolvedCwd }); | ||
| const result = await loadCapability(contextFileCapability.id, { cwd: resolvedCwd, agentDir: options.agentDir }); |
There was a problem hiding this comment.
When an SDK embedder calls the exported discoverContextFiles(cwd, profileDir) while the process-wide getAgentDir() points elsewhere, sdk/session.ts still ignores its _agentDir argument and invokes loadContextFilesInternal with only cwd. Consequently this new option remains undefined and the helper reads the global profile's AGENTS.md instead of the requested profile's file. Although createAgentSession now forwards its profile correctly, the standalone SDK discovery helper needs to forward its second argument as well.
Useful? React with 👍 / 👎.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Exact current head is — |
2d4f27f to
0884d5d
Compare
|
Exact current head is — |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
0884d5d to
7bfc6ad
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Exact current head is — |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Exact current head is — |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Exact current head is — |
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The five-axis review completed against the exact head and identified 4 actionable issues, led by Correct configured-agent write-path assertion and Removed ToolSession.home breaks existing SDK/tool integrations. These findings require changes before approval.
Findings / Required Changes
- [P1] Correct configured-agent write-path assertion.
Reference:packages/coding-agent/test/skill-management.test.ts:123
The test writesconfigured-skillbut expects a receipt path ending inmy-skill; update the expected path toconfigured-skill. - [P1] Removed ToolSession.home breaks existing SDK/tool integrations.
Reference:packages/coding-agent/src/tools/index.ts:237-238
The public ToolSession interface no longer accepts the previously supported home field, so external session/tool implementations using it fail type-checking; retain a deprecated home alias or provide a compatibility migration before removing it. - [P1] Validate native skill names before writing.
Reference:packages/coding-agent/src/extensibility/skill-management.ts:280-282
frontmatter.name is used directly in path.join, so values such as ../outside or nested separators can escape the selected skills directory. Reject path separators, . and .., and verify the resolved destination remains beneath the scope directory before mkdir/write. - [P2] Skill-management public contract documents obsolete fixed paths.
Reference:packages/coding-agent/src/extensibility/skill-management.ts:3-8
The module-level API documentation still states global ~/.gjc/agent/skills as canonical despite agentDir profiles being supported; update the contract to describe the selected agent directory and default fallback.
CI / Verification
- Reviewed the exact remote head:
1d084bc385626b302aceb86d81ab7d874241ecc9. - CI summary: 1 passing, 3 failing, 15 pending/cancelled/skipped.
- Failing checks:
Validate exact-head PR contract,Validate exact-head PR contract,PR contract bootstrap. - Repository policy permits review before all gating checks pass; the current non-passing checks are recorded above and do not establish that checks passed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | CHANGES_REQUESTED | Agent-directory propagation is mostly coherent, but removing ToolSession.home creates a source compatibility break for external SDK/tool hosts. |
| A2. Architecture / Correctness / Failure | CHANGES_REQUESTED | Concurrency ordering is deterministic in the inspected discovery paths; native skill writes retain an actionable path traversal risk. |
| A3. Security / Privacy / Trust | APPROVED | Agent-directory isolation and user-scope discovery changes do not establish a security or privacy defect. |
| A4. Verification / Tests / CI | CHANGES_REQUESTED | A4 verification is incomplete in CI and includes one confirmed failing assertion in the added configured-agent-directory test. |
| A5. Context / Compatibility / Platform | APPROVED | Agent-directory profiles are consistently threaded through discovery, runtime loading, session startup, and documentation; CI verification remains incomplete. |
Limitations
- Failed PR contract checks and skipped platform/integration jobs prevent claiming CI-verified cross-platform behavior.
|
Exact current head is — |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 similar comment
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Exact current head is — |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Exact current head is — |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Exact current head is — |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Delta review at exact head 689ffa55 — merge blocked.
Session-core/delegated Settings threading and static skill-write symlink/traversal defenses are fixed. Two HIGH compatibility/public-boundary defects remain:
- public SDK
discoverContextFiles(cwd, agentDir)andbuildSystemPrompt({agentDir})cannot carry the owning Settings. They still fall back to cwd/global last-session policy, so same-cwd profiles can authorize/suppress each other's providers/extensions and context, including after close-order changes. - the new deprecated
ToolSession.homefield is preferred over trustedgetSessionHome(). A stale compatibility value can redirect home-relative custom skill discovery and exact invocation to another profile.
Thread Settings through both public SDK wrappers and test concurrent same-cwd profiles/close orders. Use getSessionHome?.() ?? home in skill discovery/invocation, with conflicting-value regressions.
Approved native precedence, protected names, chaining, delegated profile propagation, and static write containment otherwise remain intact.
Reviewed by @probepark — finite delta review from blocked 7a35fda9.
gajae.pr-review-verdict.v1 merge-blocked sha256:d60c13292f40b0f3f0895274177962267cbd28bbe61562804a98f135d0241360 reviewer:human reviewer-id:probepark evidence:exact-head-689ffa55-public-settings-and-trusted-home-authority-gaps
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Exact current head is — |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Exact head — |
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The five-axis review completed against the exact head and identified 7 actionable issues, led by Concurrent skill writes can overwrite or interleave nondeterministically and Configured-agent skill test expects the wrong path. These findings require changes before approval.
Findings / Required Changes
- [P1] Concurrent skill writes can overwrite or interleave nondeterministically.
Reference:crates/pi-natives/src/path_identity.rs:4467-4515
Multiple writers can open the same SKILL.md, each validate and truncate it, then write without serialization or atomic replacement. Serialize per target or write a temporary descriptor and atomically replace it after validation. - [P1] Configured-agent skill test expects the wrong path.
Reference:packages/coding-agent/test/skill-management.test.ts:166-167
The test writesconfigured-userbut asserts a receipt path ending inmy-skill; update the expected path toconfigured-skillso the configured-agent behavior can pass. - [P1] User skill writes are unavailable on Windows.
Reference:crates/pi-natives/src/path_identity.rs:8728-8734
The Windows native implementation always returnsunsupported_platform, whilewriteNativeSkillnow requires this primitive and no longer has a compatible fallback; implement a reparse-safe Windows writer or explicitly preserve a supported cross-platform API contract. - [P2] Configured skill test asserts the wrong effective name.
Reference:packages/coding-agent/test/skill-management.test.ts:191
The test writes frontmatter namingmy-skillwhile requestingconfigured-skill, then expects amy-skillpath. Update the fixture or expected path so the test verifies the intended contract. - [P2] Configured-agent skill test expects the wrong skill name.
Reference:packages/coding-agent/test/skill-management.test.ts:192-193
The test writesconfigured-skillbut asserts a path containingmy-skill, so it fails independently of the implementation; assert the requested/effectiveconfigured-skillpath. - [P2] Document unsupported skill writes on Windows and non-Unix platforms.
Reference:docs/skills.md:69-83
The native writer explicitly returnsunsupported_platformoutside Unix, causing managed writes to fail closed, but the installation documentation presents the workflow without a platform caveat. Document this limitation and provide the supported manual-copy alternative. - [P2] Skill writes block the JavaScript thread.
Reference:packages/coding-agent/src/extensibility/skill-management.ts:318-326
The synchronous native binding performs directory creation, truncation, and an unbounded content write inside the caller’s thread, so large content or slow storage can stall the event loop and concurrent sessions. Provide an async blocking-pool binding or enforce a bounded write path.
CI / Verification
- Reviewed the exact remote head:
102f449cdf694183f2d0b17c442bcc1ff6000876. - CI summary: 1 passing, 2 failing, 15 pending/cancelled/skipped.
- Failing checks:
Validate exact-head PR contract,PR contract bootstrap. - Repository policy permits review before all gating checks pass; the current non-passing checks are recorded above and do not establish that checks passed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | CHANGES_REQUESTED | Agent-directory propagation is broadly consistent, but Windows skill-write compatibility is not preserved and one compatibility test is internally inconsistent. |
| A2. Architecture / Correctness / Failure | CHANGES_REQUESTED | The new skill writer is not concurrency-safe and performs blocking filesystem work synchronously; other correctness risks were not established. |
| A3. Security / Privacy / Trust | APPROVED | No concrete security or privacy defect established; skill writes and profile-scoped discovery implement containment and isolation controls. |
| A4. Verification / Tests / CI | CHANGES_REQUESTED | A4 review established a deterministic failing test assertion; CI health remains limited by reported contract failures and skipped validation jobs. |
| A5. Context / Compatibility / Platform | CHANGES_REQUESTED | Agent-directory integration and skill documentation are mostly aligned, but a test contradiction and undocumented non-Unix write limitation leave compatibility risk. |
Limitations
- PR contract bootstrap failed and affected integration, native-build, Windows, and platform validation jobs were skipped; therefore those CI validations cannot establish cross-platform pass status.
Native capability readers and writers diverged whenever a session or SDK caller selected an agent-directory profile. Thread the resolved profile through every native discovery surface while preserving explicit-home SDK isolation and current session settings authority.\n\nLore-id: 4784-recovery\nConstraint: preserve current-dev #4840/#4841 session settings and cancellation behavior\nRejected: replay stale CI and release changes | outside the user-scope discovery boundary\nConfidence: high\nScope-risk: wide\nReversibility: revertible\nTested: focused native user-scope discovery suite; coding-agent type check\nNot-tested: PR-hosted CI
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Exact head — |
4 participants
What
Native user-scope discovery resolves native user surfaces from the selected agent directory with session-local policy authority.
Exact-head evidence
e36e1048e26f8613584c94ce76de36bfc3451eedd06a42e53a9d6363d152a88c8168b5d6b2ab345eSKILL.md.—
[repo owner's gaebal-gajae (clawdbot) 🦞]